refactor(docs): reimplement interactive demos in React, delete vendored JS#277
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughSeven interactive demo simulations previously delivered as vanilla-JS iframe embeds under ChangesDemo iframe → React component migration
Sequence Diagram(s)sequenceDiagram
participant User
participant DemoModal
participant demoComponent
participant React.Suspense
participant LazyDemoComponent
participant useRafLoop
participant useReducedMotion
User->>DemoModal: open modal with demo id
DemoModal->>demoComponent: demoComponent(current.id)
demoComponent-->>DemoModal: LazyDemo (lazy import wrapper)
DemoModal->>React.Suspense: render Demo with theme prop
React.Suspense->>LazyDemoComponent: dynamic import on first render
LazyDemoComponent->>useReducedMotion: check prefers-reduced-motion
useReducedMotion-->>LazyDemoComponent: reduced: boolean
LazyDemoComponent->>useRafLoop: start RAF loop (active = !reduced)
useRafLoop-->>LazyDemoComponent: callback(now) each frame
LazyDemoComponent-->>User: animated SVG/Canvas simulation
Estimated code review effort🎯 5 (Critical) | ⏱️ ~125 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/app/components/landing/demo-modal.tsx (1)
89-103:⚠️ Potential issue | 🟠 Major | ⚡ Quick winExpand focus-trap selector for native form controls.
Now that demos render directly in the modal, the trap list misses focusable controls like
input/select/textarea(e.g., slider controls), so Tab navigation can escape the dialog or skip interactive elements.Suggested fix
const focusable = dialog.querySelectorAll<HTMLElement>( - 'a[href], button:not([disabled]), iframe, [tabindex]:not([tabindex="-1"])', + 'a[href], button:not([disabled]), input:not([disabled]), select:not([disabled]), textarea:not([disabled]), iframe, [tabindex]:not([tabindex="-1"])', );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/app/components/landing/demo-modal.tsx` around lines 89 - 103, The focus-trap selector in the useEffect hook is incomplete and does not include native form controls. Expand the querySelector selector string used in the querySelectorAll call (currently selecting only a, button, iframe, and tabindex elements) to also include input, select, and textarea elements so that all focusable form controls are properly trapped within the dialog when Tab focus navigation occurs.
🧹 Nitpick comments (3)
docs/app/components/demos/ratelimit-demo.tsx (1)
94-94: ⚡ Quick winComponent should accept
DemoPropsto match the registry contract.Per the demo-modal integration (context snippet 4),
DemoModalpasses athemeprop to all demos:<Demo theme={theme} />. This component should accept theDemoPropsinterface for type consistency, even if the prop is unused (since theming is handled via CSS variables).Suggested fix
-export default function RateLimitDemo() { +import type { DemoProps } from "./types"; + +export default function RateLimitDemo(_props: DemoProps) {Or if
DemoPropsisn't exported from types, inline it:-export default function RateLimitDemo() { +export default function RateLimitDemo(_props: { theme: string }) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/app/components/demos/ratelimit-demo.tsx` at line 94, The RateLimitDemo component function signature does not accept the DemoProps parameter, but the DemoModal integration passes a theme prop to all demo components. Update the RateLimitDemo function declaration to accept the DemoProps interface as a parameter. If DemoProps is not already exported from types, import it first, then add the parameter to match the registry contract that DemoModal expects when it renders the component with the theme prop.docs/app/components/demos/scaling-demo.tsx (1)
292-296: 💤 Low valueConsider frame-rate-independent timing for consistent behavior across devices.
The
loopcallback uses a fixed timestep (0.04sper sub-step) and ignores thenowtimestamp provided byuseRafLoop. This means the simulation runs faster on 120Hz displays and slower on throttled/30Hz devices. Other demos in this cohort (e.g.,worksteal-demo) track elapsed time for frame-rate independence.If reproducible fixed-rate behavior is intentional for this educational simulation, this is fine. Otherwise, consider tracking delta time:
♻️ Optional: frame-rate independent timing
+ const lastRef = useRef<number>(0); + - const loop = useCallback(() => { - const sub = 2; - for (let i = 0; i < sub; i++) stepSim((0.05 / sub) * 1.6); + const loop = useCallback((now: number) => { + if (!lastRef.current) lastRef.current = now; + const dt = Math.min((now - lastRef.current) / 1000, 0.05); + lastRef.current = now; + const sub = 2; + for (let i = 0; i < sub; i++) stepSim((dt / sub) * 1.6); drawAll(); }, [stepSim, drawAll]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/app/components/demos/scaling-demo.tsx` around lines 292 - 296, The loop callback uses a fixed timestep instead of frame-rate-independent timing, which causes inconsistent simulation speeds on different display refresh rates. Modify the loop callback to use the now parameter provided by useRafLoop to calculate the actual delta time elapsed since the last frame, then pass this delta time to stepSim instead of the hardcoded 0.04s value. Track the previous frame timestamp to compute the elapsed time between frames and ensure the simulation runs at consistent speed regardless of device refresh rate.docs/app/components/demos/workflow-demo.tsx (1)
222-249: ⚖️ Poor tradeoffConsider tracking dot animations for cleanup on unmount.
The dot animation uses its own RAF loop independent of React's lifecycle. If the component unmounts while dots are animating, the RAF callbacks continue until completion (up to 480ms), operating on detached elements.
This is low-impact since dots self-cleanup, but for robustness you could track active animation frames and cancel them in a cleanup effect:
♻️ Optional: Track and cancel dot animations
+const dotFramesRef = useRef<Set<number>>(new Set()); + +useEffect(() => { + return () => { + for (const frame of dotFramesRef.current) cancelAnimationFrame(frame); + }; +}, []); + const spawnDot = useCallback((key: string) => { // ... existing setup ... const move = (now: number) => { // ... existing logic ... - if (p < 1) requestAnimationFrame(move); - else dot.remove(); + if (p < 1) { + const frame = requestAnimationFrame(move); + dotFramesRef.current.add(frame); + } else { + dot.remove(); + } }; - requestAnimationFrame(move); + const frame = requestAnimationFrame(move); + dotFramesRef.current.add(frame); }, []);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/app/components/demos/workflow-demo.tsx` around lines 222 - 249, The spawnDot callback spawns requestAnimationFrame animations via the move function that can continue executing after component unmount, wasting resources on detached elements. Create a Set-based ref to track active animation frame IDs, store each requestAnimationFrame(move) return value in this tracking Set, remove completed animation frame IDs when animations finish (when p >= 1), and add a useEffect cleanup function that cancels all remaining tracked animation frames when the component unmounts using cancelAnimationFrame.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/app/components/demos/mesh-demo.tsx`:
- Around line 143-144: The token limit check in the loop uses a greater-than
comparison operator which allows the tokens array to exceed the intended maximum
by one element. Change the condition from `>` to `>=` in the comparison
`sim.tokens.length > MAX_TOKENS` to ensure the loop breaks immediately when the
token count reaches MAX_TOKENS rather than allowing it to go one beyond the
limit.
- Line 123: The MeshDemo component function signature does not accept the theme
prop that is being passed to it from DemoModal. Update the function signature of
MeshDemo to accept a destructured theme prop with the DemoProps type annotation,
changing from a function with no parameters to one that destructures the theme
property from the DemoProps interface. This will ensure the component properly
receives and can use the theme value passed by its parent.
In `@docs/app/components/demos/recovery-demo.tsx`:
- Around line 174-185: When reduced-motion mode becomes true during playback,
the RAF loop stops executing (due to the `playing && !reduced` condition in
useRafLoop at line 174), but the `playing` state remains true, leaving the UI in
a stale "Pause" state. Fix this by ensuring that when `reduced` is true in the
`startPlay` callback, you also call `setPlaying(false)` to keep the `playing`
state synchronized with the actual playback state, so the UI control reflects
the frozen timeline correctly.
In `@docs/app/components/demos/registry.ts`:
- Around line 22-24: The demoComponent function uses unchecked property access
on DEMO_COMPONENTS which can return inherited prototype properties for strings
like __proto__ instead of only own properties. Guard the lookup in demoComponent
to verify the id exists as an own property of DEMO_COMPONENTS using
Object.hasOwn() or Object.prototype.hasOwnProperty.call() before returning the
value, otherwise return undefined to maintain the documented behavior of
returning undefined for unknown ids.
In `@docs/app/components/demos/saga-demo.tsx`:
- Line 126: The SagaDemo function signature does not accept the DemoProps
parameter, which causes a type mismatch with the registry contract that expects
ComponentType<DemoProps>. Modify the SagaDemo function to accept DemoProps as a
parameter in its signature to maintain type consistency with the registry, even
if the props are not immediately used within the component.
In `@docs/app/components/demos/workflow-demo.tsx`:
- Line 203: The WorkflowDemo component does not accept the DemoProps parameter
in its function signature, which violates the ComponentType<DemoProps> contract
defined in the registry. Update the WorkflowDemo function signature to accept
props of type DemoProps as a parameter (similar to how ScalingDemo implements
it). This ensures the component matches the registry's type expectations and
properly receives the theme prop passed by DemoModal at line 185, maintaining
consistency with the type system even if the component currently relies on CSS
variables for theming.
In `@docs/app/components/demos/worksteal-demo.tsx`:
- Around line 316-317: The simulation uses wall-clock timestamps from
performance.now() to track job/token deadlines, but the pause mechanism only
stops RAF scheduling, allowing real time to advance during the pause. When
resumed after a long pause, deadlines calculated relative to the old timestamps
become stale and expire immediately. Fix this by tracking cumulative pause
duration and adjusting all timestamp-based deadline calculations throughout the
simulation. Specifically, capture the pause start time when paused becomes true
and accumulate the elapsed pause duration when paused becomes false, then offset
all performance.now() calls used in deadline calculations (such as the rs.busy
assignment and similar deadline-setting code in other locations marked in the
"Also applies to" note) by subtracting the total accumulated pause time so that
paused duration does not count toward expiration.
- Around line 465-490: The Pause/Resume button toggle does not expose its
current state to assistive technology. Add the aria-pressed attribute to the
button element (the one with className "ctl" that has onClick={() =>
setPaused((p) => !p)}) and set it equal to the paused state variable to properly
communicate the button's toggled state to screen readers and other accessibility
tools.
---
Outside diff comments:
In `@docs/app/components/landing/demo-modal.tsx`:
- Around line 89-103: The focus-trap selector in the useEffect hook is
incomplete and does not include native form controls. Expand the querySelector
selector string used in the querySelectorAll call (currently selecting only a,
button, iframe, and tabindex elements) to also include input, select, and
textarea elements so that all focusable form controls are properly trapped
within the dialog when Tab focus navigation occurs.
---
Nitpick comments:
In `@docs/app/components/demos/ratelimit-demo.tsx`:
- Line 94: The RateLimitDemo component function signature does not accept the
DemoProps parameter, but the DemoModal integration passes a theme prop to all
demo components. Update the RateLimitDemo function declaration to accept the
DemoProps interface as a parameter. If DemoProps is not already exported from
types, import it first, then add the parameter to match the registry contract
that DemoModal expects when it renders the component with the theme prop.
In `@docs/app/components/demos/scaling-demo.tsx`:
- Around line 292-296: The loop callback uses a fixed timestep instead of
frame-rate-independent timing, which causes inconsistent simulation speeds on
different display refresh rates. Modify the loop callback to use the now
parameter provided by useRafLoop to calculate the actual delta time elapsed
since the last frame, then pass this delta time to stepSim instead of the
hardcoded 0.04s value. Track the previous frame timestamp to compute the elapsed
time between frames and ensure the simulation runs at consistent speed
regardless of device refresh rate.
In `@docs/app/components/demos/workflow-demo.tsx`:
- Around line 222-249: The spawnDot callback spawns requestAnimationFrame
animations via the move function that can continue executing after component
unmount, wasting resources on detached elements. Create a Set-based ref to track
active animation frame IDs, store each requestAnimationFrame(move) return value
in this tracking Set, remove completed animation frame IDs when animations
finish (when p >= 1), and add a useEffect cleanup function that cancels all
remaining tracked animation frames when the component unmounts using
cancelAnimationFrame.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7b5d55b3-8929-4236-b683-9da463d42269
⛔ Files ignored due to path filters (1)
docs/public/demos/favicon.svgis excluded by!**/*.svg
📒 Files selected for processing (29)
.pre-commit-config.yamldocs/app/app.cssdocs/app/components/demos/index.tsdocs/app/components/demos/lib/index.tsdocs/app/components/demos/lib/use-raf-loop.tsdocs/app/components/demos/lib/use-reduced-motion.tsdocs/app/components/demos/mesh-demo.tsxdocs/app/components/demos/ratelimit-demo.tsxdocs/app/components/demos/recovery-demo.tsxdocs/app/components/demos/registry.tsdocs/app/components/demos/saga-demo.tsxdocs/app/components/demos/scaling-demo.tsxdocs/app/components/demos/types.tsdocs/app/components/demos/workflow-demo.tsxdocs/app/components/demos/worksteal-demo.tsxdocs/app/components/landing/demo-modal.tsxdocs/app/styles/demos.cssdocs/app/styles/landing.cssdocs/public/demos/README.mddocs/public/demos/demo-mesh.jsdocs/public/demos/demo-playground.jsdocs/public/demos/demo-ratelimit.jsdocs/public/demos/demo-recovery.jsdocs/public/demos/demo-saga.jsdocs/public/demos/demo-workflow.jsdocs/public/demos/demo-worksteal.jsdocs/public/demos/interactive.cssdocs/public/demos/interactive.htmldocs/public/demos/theme.css
💤 Files with no reviewable changes (12)
- docs/public/demos/README.md
- docs/public/demos/demo-ratelimit.js
- docs/app/styles/landing.css
- docs/public/demos/interactive.html
- docs/public/demos/demo-playground.js
- docs/public/demos/interactive.css
- docs/public/demos/demo-workflow.js
- docs/public/demos/demo-mesh.js
- docs/public/demos/demo-saga.js
- docs/public/demos/demo-recovery.js
- docs/public/demos/theme.css
- docs/public/demos/demo-worksteal.js
What
Replaces the vendored vanilla micro-app under
docs/public/demos/with native React components rendered directly inside the homepage demo modal. Deletes the iframe entirely.Follows
tasks/react-demos-plan.md.Why
The modal previously embedded
interactive.html?embed=<id>in an<iframe>— duplicate theme/accent plumbing (postMessage, URL params), a second CSS system (~515 lines), and untyped DOM JS outside lint/tsc. The React port is one type-checked + biome-linted codebase, theme viauseThemeMode(), no static-asset shipping.Changes
app/components/demos/—types.ts,registry.ts(lazy per-id),lib/{use-raf-loop,use-reduced-motion}.ts, barrels (index.ts,lib/index.ts).Suspensestage; iframe +srcbuilder +loadingstate removed; focus trap / Escape / scroll-lock / restore preserved.saga,mesh,recovery,ratelimit,workflow,worksteal,scaling(Canvas 2D). RAF viauseRafLoop; reduced-motion static frames; a11y (aria-pressed, roles, keyboard) preserved.styles/demos.css: shared demo chrome scoped under.dm-stage(tokens already intokens.css).docs/public/demos/(vendored JS/HTML/CSS) + the now-unused.dm-frameiframe CSS.Verify
pnpm typecheck+pnpm lint+pnpm buildgreen./demos/no longer emitted in the build output.Notes
Summary by CodeRabbit
Release Notes
New Features
Chores